Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remix example bugfixes #987

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: remix example bugfixes #987

wants to merge 3 commits into from

Conversation

eric-schmidt
Copy link

@eric-schmidt eric-schmidt commented Dec 10, 2024

Description

The Remix example in this repo does not currently work out-of-the-box. I believe I have pinned this down to two issues. First, even though lib/contentful.server.ts is marked as a server-only file, it is somehow making it into the bundle delivered to the client, and the client chokes on references to process.env.CONTENTFUL_SPACE_ID, since this var is only available in Node. Second, certain browsers (Safari and Arc, in my experience) have trouble properly setting the preview mode cookie — even though it is properly setting sameSite: ‘none' and secure: true, which has alleviated the issue for me in the past when doing something similar for Next.js Draft Mode.

The client vs. server variable issue has been addressed by refactoring how the Contentful client is instantiated, instead grabbing any process.env vars in the loader function, which only runs on the server, and passing directly to lib/contentful.server.ts. However, it is worth noting that this feels like a Remix bug, since according to the docs anything with a .server.ts extension should never run on the client 🤔

The cookie issue may not be able to be addressed, as any browser with strict privacy settings enabled will likely run into this issue. Perhaps it would be good to add some notes to the main Live Preview repo README and/or the Live Preview docs noting this caveat? This is briefly mentioned in our Live Preview docs in regards to authentication cookies, but it might be worth calling out more explicitly that this could have ramifications for various, cookie-dependent “draft modes” as well (i.e. Remix and Next.js).

Changes

  • Moves types to ./examples/remix/types.d.ts, making them reusable throughout the Remix example codebase.
  • Refactors ./examples/remix/lib/contentful.server.ts, allowing function parameters to be passed in from loader functions, which DO have access to process.env variables.
  • Updates ./examples/remix/app/routes/$slug.tsx, ./examples/remix/app/routes/preview.ts, and ./examples/remix/app/routes/index.tsx to take advantage of updated Contentful client instantiation in ./examples/remix/lib/contentful.server.tx.
  • Fixes improper DOM nesting in ./examples/remix/app/components/preview-banner.tsx (<form> elements cannot be nested with <p> elements).

@eric-schmidt eric-schmidt changed the title Fix/remix example fix: remix example bugfixes Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant